bpo-37064: add option -k to Tools/scripts/pathfix.py by PatrikKopkan · Pull Request #15548 · python/cpython

PatrikKopkan · 2019-08-27T12:51:08Z

with this option it is possible to keep flags when changing shebang line

https://bugs.python.org/issue37064

- with this option it is possible to keep flags when changing shebang line

vstinner · 2019-08-29T15:27:41Z

 new_interpreter = None
 preserve_timestamps = False
 create_backup = True
+add_flag = None


This variable looks unused.

vstinner · 2019-08-29T15:28:47Z

+
+def parse_shebang(shebangline):
+    end = len(shebangline)
+    start = shebangline.find(b' -', 0, end)  # .find() returns index at space


Please remove the comment, IMHO it's not really helpful.

", 0, end" are the default value and so can be removed.

vstinner · 2019-08-29T15:29:27Z


+
+def parse_shebang(shebangline):
+    end = len(shebangline)


Please add a comment that on Linux, the first whitespace (U+0020) indicates the beginning of options.

vstinner · 2019-08-29T15:29:44Z

+    start = shebangline.find(b' -', 0, end)  # .find() returns index at space
+    if start == -1:
+        return b'', b''
+    print(shebangline[(start + 2):end])


Don't commit debug print please (remove it).

vstinner · 2019-08-29T15:30:10Z

+
+
 def fixline(line):
+    global keep_flags


I don't think that this global statement is useful. Variables are global by default.

vstinner · 2019-08-29T15:32:34Z

+    if keep_flags:
+        flags = parse_shebang(line)
+        return b'#! ' + new_interpreter + b" -" + flags
    return b'#! ' + new_interpreter + b'\n'


I would prefer to have:

if keep_flags: flags = parse_shebang(line) else: flags = b'' return b'#! ' + new_interpreter + flags + b'\n'

So parse_shebang() must remove the trailing newline.

Ping @PatrikKopkan?

I have zero idea how I deleted. In editor I still have:
return b'#! ' + new_interpreter + b' -' + flags + b'\n'

vstinner · 2019-08-29T15:33:01Z

+        subprocess.call([sys.executable, self.script, '-i', '/usr/bin/python', '-n', self.temp_file])
+        with open(self.temp_file) as f:
+            output = f.read()
+        self.assertEqual(output, '#! /usr/bin/python\n' + 'print("Hello world")\n')


Please add a test using -k when the shebang does not contain any flag.

vstinner · 2019-08-29T15:38:35Z

+        self.temp_file = support.TESTFN
+
+        with open(self.temp_file, 'w') as f:
+            f.write('#! /usr/bin/env python -R\n' + 'print("Hello world")\n')


I would prefer to have an helper method which takes two arguments: (shebang, pathfix_flags) and return the new shebang. To get tests which look like:

def test_keep_flags(self): shebang = '#! /usr/bin/env python -R' self.assertEqual( self.pathfix(shebang, ['-i', '/usr/bin/python']), '#! /usr/bin/python') self.assertEqual( self.pathfix(shebang, ['-i', '/usr/bin/python', '-k']), '#! /usr/bin/python -R')

vstinner · 2019-08-29T15:40:01Z

+
+    @classmethod
+    def setupUpClass(cls):
+        self.addCleanup(support.unlink, support.TESTFN)


It doesn't work: self doesn't exist. You may use setUp().

vstinner · 2019-08-29T15:41:10Z

@@ -0,0 +1 @@
+Add flag -k to pathscript.py. This flag preserve flags from shebang before change and adds them after change.


I suggest: "Add flag -k to pathscript.py script: preserve shebang flags."

bedevere-bot · 2019-08-29T15:42:30Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2019-09-04T09:08:35Z

@@ -0,0 +1,48 @@
+import unittest, os, subprocess


Please put one import per line: see PEP 8.

Try also to sort imports.

vstinner · 2019-09-04T09:10:16Z

+        subprocess.call([sys.executable, self.script, *pathfix_flags, self.temp_file])
+        with open(self.temp_file) as f:
+            output = f.read()
+        print(output)


Don't forget to remove debug print when you submit a PR.

vstinner · 2019-09-04T09:11:27Z

+        self.addCleanup(support.unlink, support.TESTFN)
+
+    def pathfix(self, shebang, pathfix_flags):
+        with open(self.temp_file, 'w') as f:


I would prefer to specify the encoding: pass encoding="utf8" to this open, and the other one below.

vstinner · 2019-09-04T09:12:53Z

+        self.assertEqual(
+            self.pathfix(
+                '#! /usr/bin/env python -R',
+                ['-i', '/usr/bin/python', '-k', '-n']),


Please remove -n from tests and always put it in pathfix() function.

vstinner · 2019-09-04T09:14:37Z

+            self.pathfix(
+                '#! /usr/bin/env python -R',
+                ['-i', '/usr/bin/python', '-k', '-n']),
+            '#! /usr/bin/python -R\n' + 'print("Hello world")\n'


I don't think that it's useful to check for 'print("Hello world")\n' in each test. I would prefer to test it in pathfix() and just return the first line.

Something like this in pathfix():

lines = output.split('\n')
self.assertEqual(lines[1:], ['print("Hello world")\n']
shebang = lines[0][:-1] # strip the newline
return shebang

vstinner

Thanks, the code now looks way better! A few more comments.

vstinner · 2019-09-05T11:06:15Z

+            shebang = lines[0][:-1]  # strip the newline
+            return shebang
+
+    def test_pathfix_keeping_flags(self):


I would prefer:

put test_pathfix_without_keeping_flags() code into test_pathfix()

move test_pathfix_keeping_flags() after test_pathfix()

add a test in test_pathfix_keeping_flags(): use -k but the shebang doesn't contain any flag (the bug that you just fixed)

vstinner · 2019-09-05T11:06:50Z

+def parse_shebang(shebangline):
+    shebangline = shebangline.strip()
+    end = len(shebangline)
+    start = shebangline.find(b' -', 0, end)


"end" argument is useless: shebangline.find(b' -') behaves the same and is simpler. Remove 0 and end.

vstinner · 2019-09-05T11:07:09Z

+    if keep_flags:
+        flags = parse_shebang(line)
+        return b'#! ' + new_interpreter + b" -" + flags
    return b'#! ' + new_interpreter + b'\n'


Ping @PatrikKopkan?

vstinner

LGTM.

Add flag -k to pathscript.py script: preserve shebang flags.

* bpo-37064: Add option -k to Tools/scripts/pathfix.py (GH-15548) Add flag -k to pathscript.py script: preserve shebang flags. (cherry picked from commit 50254ac) * bpo-37064: Add option -a to pathfix.py tool (GH-15717) Add option -a to Tools/Scripts/pathfix.py script: add flags. (cherry picked from commit 1dc1acb)

Add flag -k to pathscript.py script: preserve shebang flags.

bpo-37064: add option -k to Tools/scripts/pathfix.py

b1cbcc1

- with this option it is possible to keep flags when changing shebang line

the-knights-who-say-ni added the CLA signed label Aug 27, 2019

bedevere-bot added the awaiting review label Aug 27, 2019

vstinner requested changes Aug 29, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Aug 29, 2019

bedevere-bot added the awaiting changes label Aug 29, 2019

Patrik Kopkan added 2 commits September 2, 2019 16:52

- add helper test function

48e07a3

strip shebang in parse_shebang

7f5fcbc

vstinner reviewed Sep 4, 2019

View reviewed changes

little fixes

25d361f

vstinner requested changes Sep 5, 2019

View reviewed changes

Patrik Kopkan added 3 commits September 5, 2019 16:00

'adress Victor's comments'

f062817

check exit codes

0a5d298

cleanup

6e79ff6

vstinner approved these changes Sep 5, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 5, 2019

use rstrip

98612cf

vstinner merged commit 50254ac into python:master Sep 5, 2019

bedevere-bot removed the awaiting merge label Sep 5, 2019

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019

bpo-37064: Add option -k to Tools/scripts/pathfix.py (pythonGH-15548)

7037a80

Add flag -k to pathscript.py script: preserve shebang flags.

vstinner mentioned this pull request Sep 25, 2019

[3.8] bpo-37064: Add -k and -a options to pathfix.py tool #16387

Merged

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020

bpo-37064: Add option -k to Tools/scripts/pathfix.py (pythonGH-15548)

640c675

Add flag -k to pathscript.py script: preserve shebang flags.

websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020

bpo-37064: Add option -k to Tools/scripts/pathfix.py (pythonGH-15548)

1061309

Add flag -k to pathscript.py script: preserve shebang flags.

		@@ -0,0 +1 @@
		Add flag -k to pathscript.py. This flag preserve flags from shebang before change and adds them after change. No newline at end of file

Uh oh!

Uh oh!

Conversation

PatrikKopkan commented Aug 27, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PatrikKopkan commented Aug 27, 2019 •

edited by bedevere-bot

Loading